Skip to content

Working FX handling#170

Merged
mattiarighi merged 64 commits into
developmentfrom
development_fx_RESTRUCTURED_Updated
Sep 30, 2019
Merged

Working FX handling#170
mattiarighi merged 64 commits into
developmentfrom
development_fx_RESTRUCTURED_Updated

Conversation

@valeriupredoi

@valeriupredoi valeriupredoi commented Jul 17, 2019

Copy link
Copy Markdown
Contributor

OK chaps @bouweandela @zklaus @ledm here is the long promised implementation of fx vars handling: this is a sister PR of #21 but with much improved user interface and functionality. It does the following:

  • allows for use of fx variables (CMIP5 and 6) as regular variables;
  • removes any special file retrieval functionality for fx files; the regulat get_input_filelist is used; also removes the special entries for fx files/dirs in config-developer;
  • removes the need for the user to specify the fx_files dictionary in the variable (see NOTE below);
  • reverts a bunch of changes I made previously;

NOTE:

  • if a user wants to use fx files in the diagnostic, all they have to do is include the fx variables in the variables list; they will get the fx files at the end of preprocessing, with any preprocessor of their choice applied to them (and CMOR checked and fixed);
  • if a user wants to use mask_landsea or mask_landseaice, all they have to do is to set these preproc steps in the preprocessor; no more need to specify the fx_files in variabl; the code will automatically grab the fx files for masking BUT it will apply the masks from those files WITHOUT doing any CMOR checks/fixes or any preprocessor on the fx files (basically what we had before). The next step will be to implement this functionality (allow preprocessing on fx files that are used in the preprocessor) but that is absolutely non-trival.

@valeriupredoi

Copy link
Copy Markdown
Contributor Author

good call @bouweandela we should put this in a new PR 🍺

@valeriupredoi

Copy link
Copy Markdown
Contributor Author

OK @mattiarighi what's the verdikt? 👍 or 👎 ?

@mattiarighi

Copy link
Copy Markdown
Contributor

@bouweandela dismissed his review, so you have to wait for his approval...

@valeriupredoi

Copy link
Copy Markdown
Contributor Author

@bouweandela ye olde gnashgab, what say you? 🍺

Comment thread esmvalcore/_recipe.py Outdated
@bouweandela

Copy link
Copy Markdown
Member

If you don't want to solve d2898db#issuecomment-531160887 in this pull request, can you at least make an issue for it?

@valeriupredoi

Copy link
Copy Markdown
Contributor Author

If you don't want to solve d2898db#issuecomment-531160887 in this pull request, can you at least make an issue for it?

yes, I will PR the feathers out of it in a different PR, way too much stuff in this one

@mattiarighi

Copy link
Copy Markdown
Contributor

yes, I will PR the feathers out of it in a different PR, way too much stuff in this one

I opened issue #250 so that we won't forget.

@valeriupredoi

Copy link
Copy Markdown
Contributor Author

yes, I will PR the feathers out of it in a different PR, way too much stuff in this one

I opened issue #250 so that we won't forget.

cheers @mattiarighi 🍺

@valeriupredoi

valeriupredoi commented Sep 30, 2019 via email

Copy link
Copy Markdown
Contributor Author

@valeriupredoi

valeriupredoi commented Sep 30, 2019 via email

Copy link
Copy Markdown
Contributor Author

@mattiarighi

Copy link
Copy Markdown
Contributor

Done. Waiting for final approval...

@valeriupredoi

valeriupredoi commented Sep 30, 2019 via email

Copy link
Copy Markdown
Contributor Author

@mattiarighi mattiarighi merged commit f2da60c into development Sep 30, 2019
@mattiarighi mattiarighi deleted the development_fx_RESTRUCTURED_Updated branch September 30, 2019 14:50
@mattiarighi

Copy link
Copy Markdown
Contributor

@valeriupredoi please close the related issues.

@valeriupredoi

valeriupredoi commented Sep 30, 2019 via email

Copy link
Copy Markdown
Contributor Author

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants